feat: Add Memo<T> node type for skipping diff on unchanged data#51
feat: Add Memo<T> node type for skipping diff on unchanged data#51MCGPPeters wants to merge 2 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Memo<T> virtual-DOM node wrapper intended to skip diffing subtrees when the associated data hasn’t changed, reducing diff cost for large stable lists.
Changes:
- Introduces
IMemoNode+Memo<T>in DOM layer and hooks memo unwrapping intoDiffInternal, HTML rendering, and key extraction. - Adds
Elements.memo<T>(...)helpers for creating memo-wrapped nodes. - Adds unit tests covering memo diffing/rendering scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| Abies/Html/Elements.cs | Adds memo<T> helper overloads and XML docs for creating memo-wrapped nodes. |
| Abies/DOM/Operations.cs | Introduces IMemoNode/Memo<T> and integrates memo unwrapping into rendering/diffing/keying. |
| Abies.Tests/DomBehaviorTests.cs | Adds memo-focused tests and minor formatting/cleanup. |
| public interface IMemoNode | ||
| { | ||
| /// <summary> | ||
| /// The underlying node that will be rendered. | ||
| /// </summary> | ||
| Node InnerNode { get; } | ||
|
|
||
| /// <summary> | ||
| /// Checks if this memo node has the same data as another memo node. | ||
| /// Returns false if the other node is not a compatible IMemoNode. | ||
| /// </summary> | ||
| bool HasSameData(IMemoNode other); |
There was a problem hiding this comment.
Render.Html now unwraps IMemoNode, but other framework traversals still operate only on Element (e.g., handler registration/unregistration and ID preservation). As a result, memo-wrapped subtrees can render handler attributes into HTML but never get their handlers registered, breaking event dispatch. Memo support should include unwrapping in all DOM-walkers that assume Node is an Element tree.
| [Fact] | ||
| public void Memo_WithSameData_SkipsDiffing() | ||
| { | ||
| // Create two memo nodes with the same data | ||
| var row = new TestRow(1, "Test"); | ||
| var element = new Element("row-1", "tr", [], new Text("t1", "Test")); | ||
|
|
||
| var oldNode = new Memo<TestRow>(row, element); | ||
| var newNode = new Memo<TestRow>(row, element); | ||
|
|
||
| var patches = Operations.Diff(oldNode, newNode); | ||
|
|
||
| // Should produce no patches because data is equal | ||
| Assert.Empty(patches); | ||
| } |
There was a problem hiding this comment.
Memo_WithSameData_SkipsDiffing is not a strong regression test for memo diff-skipping: it diffs two Memo roots and reuses the same Element instance, which could yield no patches even without a correct memo implementation. Consider embedding memo nodes as children of a real root Element (matching production usage) and using distinct but equivalent inner node instances so the test fails if memo unwrapping/skipping is removed or broken.
| [Fact] | ||
| public void Memo_ListReorder_SkipsDiffingUnchangedItems() | ||
| { | ||
| // Simulate a list reorder where items don't change, just move | ||
| var rows = new[] | ||
| { | ||
| new TestRow(1, "First"), | ||
| new TestRow(2, "Second"), | ||
| new TestRow(3, "Third") | ||
| }; | ||
|
|
||
| // Create memo-wrapped elements for old tree | ||
| var oldChildren = rows.Select((row, i) => | ||
| (Node)new Memo<TestRow>(row, new Element($"row-{row.Id}", "tr", [], new Text($"t{row.Id}", row.Label))) | ||
| ).ToArray(); | ||
| var oldTree = new Element("tbody", "tbody", [], oldChildren); | ||
|
|
||
| // Reorder: swap first and last | ||
| var reorderedRows = new[] { rows[2], rows[1], rows[0] }; | ||
| var newChildren = reorderedRows.Select((row, i) => | ||
| (Node)new Memo<TestRow>(row, new Element($"row-{row.Id}", "tr", [], new Text($"t{row.Id}", row.Label))) | ||
| ).ToArray(); | ||
| var newTree = new Element("tbody", "tbody", [], newChildren); | ||
|
|
||
| var patches = Operations.Diff(oldTree, newTree); | ||
|
|
||
| // The memo optimization means we should NOT see UpdateText patches | ||
| // because the data is equal and diffing was skipped for the inner content. | ||
| // We may see structural patches (AddChild, RemoveChild) for reordering. | ||
| Assert.DoesNotContain(patches, p => p is UpdateText); | ||
| } |
There was a problem hiding this comment.
Memo_ListReorder_SkipsDiffingUnchangedItems currently only asserts the absence of UpdateText patches. With the current diff algorithm, a reorder produces only structural patches (or, if memo children are accidentally ignored, no patches at all), so this test can pass even when memo support is broken. Please assert the expected structural behavior too (e.g., patches contain the expected Add/Remove/Move operations, or apply patches and compare final HTML).
| /// <example> | ||
| /// // Without memo: diffs every row on every render | ||
| /// model.Rows.Select(row => TableRow(row)) | ||
| /// | ||
| /// // With memo: only diffs rows whose data changed | ||
| /// model.Rows.Select(row => memo(row, r => TableRow(r))) |
There was a problem hiding this comment.
The XML doc example is not valid XML documentation markup as-is (plain code/comments inside <example>). Consider wrapping the sample in <code> (and escaping </> if present) so it renders correctly in generated docs/IntelliSense.
| /// <example> | |
| /// // Without memo: diffs every row on every render | |
| /// model.Rows.Select(row => TableRow(row)) | |
| /// | |
| /// // With memo: only diffs rows whose data changed | |
| /// model.Rows.Select(row => memo(row, r => TableRow(r))) | |
| /// <example> | |
| /// <code> | |
| /// // Without memo: diffs every row on every render | |
| /// model.Rows.Select(row => TableRow(row)) | |
| /// | |
| /// // With memo: only diffs rows whose data changed | |
| /// model.Rows.Select(row => memo(row, r => TableRow(r))) | |
| /// </code> |
| /// The node is only re-rendered when the underlying data changes. | ||
| /// This is similar to React.memo or Elm's lazy. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type of the data used to create the node.</typeparam> | ||
| /// <param name="Data">The data that was used to create the wrapped node.</param> | ||
| /// <param name="InnerNode">The actual DOM node (will be rendered lazily or reused).</param> | ||
| /// <remarks> | ||
| /// When diffing, if two Memo nodes have equal Data (via Equals), | ||
| /// the inner nodes are assumed to be identical and no diff is performed. | ||
| /// This dramatically reduces diffing overhead for large lists of stable items. |
There was a problem hiding this comment.
Memo<T> XML docs claim the node is "only re-rendered when the underlying data changes" and that the inner node "will be rendered lazily or reused", but the current API always constructs InnerNode eagerly (e.g., Elements.memo(data, render) calls render(data) every render). Please adjust the documentation to match actual behavior (skips diffing only), or change the implementation so it actually avoids re-rendering/allocation when data is equal.
| /// The node is only re-rendered when the underlying data changes. | |
| /// This is similar to React.memo or Elm's lazy. | |
| /// </summary> | |
| /// <typeparam name="T">The type of the data used to create the node.</typeparam> | |
| /// <param name="Data">The data that was used to create the wrapped node.</param> | |
| /// <param name="InnerNode">The actual DOM node (will be rendered lazily or reused).</param> | |
| /// <remarks> | |
| /// When diffing, if two Memo nodes have equal Data (via Equals), | |
| /// the inner nodes are assumed to be identical and no diff is performed. | |
| /// This dramatically reduces diffing overhead for large lists of stable items. | |
| /// Memo nodes allow the diff algorithm to skip comparing inner nodes when the underlying data is unchanged. | |
| /// This is conceptually similar to React.memo or Elm's lazy diffing semantics. | |
| /// </summary> | |
| /// <typeparam name="T">The type of the data used to create the node.</typeparam> | |
| /// <param name="Data">The data that was used to create the wrapped node.</param> | |
| /// <param name="InnerNode">The actual DOM node associated with this memoized value.</param> | |
| /// <remarks> | |
| /// When diffing, if two Memo nodes have equal <paramref name="Data"/> (via <see cref="object.Equals(object?, object?)"/>), | |
| /// the inner nodes are assumed to be equivalent and no diff is performed between them. | |
| /// This reduces diffing overhead for large lists of stable items, but does not by itself control when or how | |
| /// <paramref name="InnerNode"/> instances are created. |
| // Unwrap Memo nodes to get the inner node's key | ||
| if (node is IMemoNode memo) | ||
| { | ||
| node = memo.InnerNode; | ||
| } | ||
|
|
There was a problem hiding this comment.
GetKey unwraps a memo node, but DiffChildrenCore still treats Node values as Element/RawHtml/Text when generating Add/Remove patches. If list children are Memo<T> (the expected usage), the reorder/membership paths will generate no structural patches because memo nodes won't match those type checks, leaving the real DOM out of sync. Consider unwrapping memo nodes (recursively) before all child-type switches used for patch creation (remove/add/diff), e.g. via a shared UnwrapMemo(Node) helper.
| // Fast path: neither is a Memo node | ||
| if (oldNode is not IMemoNode && newNode is not IMemoNode) | ||
| { | ||
| return (oldNode, newNode, false); | ||
| } | ||
|
|
||
| // Both are Memo nodes - check if data is equal | ||
| if (oldNode is IMemoNode oldMemo && newNode is IMemoNode newMemo) | ||
| { | ||
| if (oldMemo.HasSameData(newMemo)) | ||
| { | ||
| // Data is equal - skip diffing entirely | ||
| return (oldMemo.InnerNode, newMemo.InnerNode, true); | ||
| } | ||
| // Data changed - need to diff the inner nodes | ||
| return (oldMemo.InnerNode, newMemo.InnerNode, false); | ||
| } | ||
|
|
||
| // Only one is Memo - unwrap it and continue diffing | ||
| if (oldNode is IMemoNode oldMemoOnly) | ||
| { | ||
| return (oldMemoOnly.InnerNode, newNode, false); | ||
| } | ||
|
|
||
| if (newNode is IMemoNode newMemoOnly) | ||
| { | ||
| return (oldNode, newMemoOnly.InnerNode, false); | ||
| } | ||
|
|
||
| // Shouldn't reach here, but return unchanged | ||
| return (oldNode, newNode, false); |
There was a problem hiding this comment.
UnwrapMemoNodes only unwraps a single memo layer. If a memo node wraps another memo node (easy to do via the memo(data, Node node) overload), DiffInternal will continue with oldNode/newNode still being memo nodes and then fall through without producing correct patches. Please unwrap memo nodes recursively/iteratively until the underlying non-memo Node is reached.
| // Fast path: neither is a Memo node | |
| if (oldNode is not IMemoNode && newNode is not IMemoNode) | |
| { | |
| return (oldNode, newNode, false); | |
| } | |
| // Both are Memo nodes - check if data is equal | |
| if (oldNode is IMemoNode oldMemo && newNode is IMemoNode newMemo) | |
| { | |
| if (oldMemo.HasSameData(newMemo)) | |
| { | |
| // Data is equal - skip diffing entirely | |
| return (oldMemo.InnerNode, newMemo.InnerNode, true); | |
| } | |
| // Data changed - need to diff the inner nodes | |
| return (oldMemo.InnerNode, newMemo.InnerNode, false); | |
| } | |
| // Only one is Memo - unwrap it and continue diffing | |
| if (oldNode is IMemoNode oldMemoOnly) | |
| { | |
| return (oldMemoOnly.InnerNode, newNode, false); | |
| } | |
| if (newNode is IMemoNode newMemoOnly) | |
| { | |
| return (oldNode, newMemoOnly.InnerNode, false); | |
| } | |
| // Shouldn't reach here, but return unchanged | |
| return (oldNode, newNode, false); | |
| // Iteratively unwrap memo nodes on both sides until we reach the underlying non-memo nodes. | |
| // While unwrapping, preserve the optimization of skipping the diff when both memo nodes | |
| // report equal data via HasSameData. | |
| var currentOld = oldNode; | |
| var currentNew = newNode; | |
| while (true) | |
| { | |
| // Both are Memo nodes - check if data is equal | |
| if (currentOld is IMemoNode oldMemo && currentNew is IMemoNode newMemo) | |
| { | |
| if (oldMemo.HasSameData(newMemo)) | |
| { | |
| // Data is equal - skip diffing entirely, but still return the inner nodes | |
| return (oldMemo.InnerNode, newMemo.InnerNode, true); | |
| } | |
| // Data changed - unwrap one level on both sides and continue | |
| currentOld = oldMemo.InnerNode; | |
| currentNew = newMemo.InnerNode; | |
| continue; | |
| } | |
| // Only the old node is a Memo - unwrap and continue | |
| if (currentOld is IMemoNode oldMemoOnly) | |
| { | |
| currentOld = oldMemoOnly.InnerNode; | |
| continue; | |
| } | |
| // Only the new node is a Memo - unwrap and continue | |
| if (currentNew is IMemoNode newMemoOnly) | |
| { | |
| currentNew = newMemoOnly.InnerNode; | |
| continue; | |
| } | |
| // Neither side is a Memo node anymore - we're fully unwrapped | |
| return (currentOld, currentNew, false); | |
| } |
| // Data is equal - skip diffing entirely | ||
| return (oldMemo.InnerNode, newMemo.InnerNode, true); | ||
| } |
There was a problem hiding this comment.
Memo diff-skipping currently depends only on HasSameData(). If Data is equal but the InnerNode's type/tag/id differs (or for Text/RawHtml, if the id changes), skipping the diff can leave the real DOM inconsistent because no Replace/Update patches will be emitted. To keep correctness, require at least InnerNode.Id equality and the same node shape/type before returning skipDiff=true (otherwise unwrap and continue diffing).
| // Data is equal - skip diffing entirely | |
| return (oldMemo.InnerNode, newMemo.InnerNode, true); | |
| } | |
| // Data is equal - only skip diffing if the inner node identity and shape are also stable. | |
| // This ensures the real DOM remains consistent even when memo data stays the same | |
| // but the underlying node type or Id would otherwise require a Replace/Update patch. | |
| var oldInner = oldMemo.InnerNode; | |
| var newInner = newMemo.InnerNode; | |
| // Id must be equal for safe diff skipping. | |
| if (!string.Equals(oldInner.Id, newInner.Id, StringComparison.Ordinal)) | |
| { | |
| return (oldInner, newInner, false); | |
| } | |
| // Node shape/type must be the same to avoid mismatched DOM structures. | |
| if (oldInner.GetType() != newInner.GetType()) | |
| { | |
| return (oldInner, newInner, false); | |
| } | |
| // Data, Id, and node shape are all equal - safe to skip diffing entirely. | |
| return (oldInner, newInner, true); | |
| } |
Implements memoization to skip diffing when element data hasn't changed: - Add IMemoNode interface with DataEquals() for type-safe comparison - Add Memo<T> record to wrap nodes with data for equality comparison - Add memo<T>() helper function in Elements.cs - Update DiffInternal to skip diffing when memo data matches - Add UnwrapMemoNodes helper to extract inner nodes and check equality - Update Render.Html and GetKey to handle IMemoNode This optimization allows the framework to skip the entire diff subtree when the underlying data is the same, reducing patch generation overhead for unchanged elements during reorder operations. Closes #49
1845e1a to
29889bc
Compare
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark 'Rendering Engine Throughput'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.
| Benchmark suite | Current: 8855684 | Previous: 7850fba | Ratio |
|---|---|---|---|
Abies.Benchmarks.Diffing/SmallDomDiff |
536.6434462229411 ns (± 1.621570972794626) |
494.8377917607625 ns (± 1.110715084276751) |
1.08 |
Abies.Benchmarks.Diffing/LargeDomDiff |
589.2071933746338 ns (± 1.4754873613824988) |
527.1427401029147 ns (± 2.080621126147704) |
1.12 |
This comment was automatically generated by workflow using github-action-benchmark.
CC: @MCGPPeters
📝 Description
What
Implements
Memo<T>node type for skipping diffing when element data hasn't changed.Why
When re-rendering large lists, most elements often remain unchanged. By tracking the data used to render each element, we can skip the entire diff subtree when the data matches.
How
IMemoNodeinterface withHasSameData()for type-safe comparisonMemo<T>record to wrap nodes with data for equality checkingmemo<T>()helper function for convenient usageDiffInternalto check memo data equality and skip diffing when data matches🔗 Related Issues
Fixes #49
✅ Type of Change
🧪 Testing
Test Coverage
Testing Details
Added 5 new unit tests:
Memo_SkipsDiffing_WhenDataEqual- Verifies no patches when data unchangedMemo_DiffsContent_WhenDataDifferent- Verifies patches when data changesMemo_SkipsDiffing_ForReorderedChildren- Verifies memo works during reorderMemo_WorksWithRenderNode- Verifies render function is called correctlyMemo_ShouldRenderInnerNode- Verifies HTML rendering worksAll 90 tests pass.
📸 Usage Example
✨ Changes Made
IMemoNodeinterface withData,InnerNode, andHasSameData()Memo<T>record implementingIMemoNodememo<T>()helper inElements.csUnwrapMemoNodes()helper in diffing logicDiffInternalto handle memo nodesRender.HtmlandGetKeyto unwrap memo nodes🔍 Code Review Checklist
🚀 Deployment Notes
None
📋 Additional Context
Current Limitations
The memo optimization provides the most benefit when combined with the LIS optimization (#50). In the current architecture:
DiffInternal, so memo can't helpFuture Improvements
Once #50 is merged, memo will provide additional benefits:
Thank you for contributing to Abies! 🌲